Skip to content

Prepare workflows for the zizmor bump in #360 - #365

Merged
robzolkos merged 2 commits into
basecamp:mainfrom
robzolkos:ci/zizmor-prep-for-action-bumps
Jul 10, 2026
Merged

Prepare workflows for the zizmor bump in #360#365
robzolkos merged 2 commits into
basecamp:mainfrom
robzolkos:ci/zizmor-prep-for-action-bumps

Conversation

@robzolkos

@robzolkos robzolkos commented Jul 10, 2026

Copy link
Copy Markdown
Collaborator

#360 bumps zizmor-action v0.5.3 → v0.5.7, which ships a newer zizmor (1.26.x). That version reports five findings the currently pinned zizmor does not, so #360 fails GitHub Actions audit for reasons unrelated to the twenty actions it bumps. zizmor exits non-zero on any finding, so all five must be resolved.

Four of the five are not #360's doing. This clears them on main first, so #360 can go green on a plain rebase without anyone hand-editing a dependabot branch. Of the four, three are fixed, one is removed at the source, and exactly one is suppressed.

adhoc-packages in the publish job — removed, not suppressed

release-typescript.yml ran npm install -g npm@latest: an unpinned install from the registry, inside the publish job that holds id-token: write and the release-npm environment, three steps before npm publish of @37signals/basecamp.

The blast radius is not "a linter is unhappy." A compromised npm@latest there executes against a live OIDC publishing identity and can push a malicious release of this SDK. Pinning the version would narrow that, but zizmor still flags it — the rule is about installing outside a lockfile at all — so a pin buys a suppression, not a fix.

Node 24 bundles npm 11.16, past the 11.5.1 that OIDC trusted publishing requires. The step is simply unnecessary. The publish job now uses Node 24 and takes npm from the Node tarball, which setup-node verifies by checksum. The ad-hoc install is gone rather than accepted.

Verified locally under Node 24 (npm 11.16.0): npm ci, npm run build (what prepublishOnly runs, including postbuild), and the full suite — 616 tests across 64 files, all passing.

adhoc-packages in the bundler-audit job — suppressed, with reason

gem install bundler-audit remains. That job has contents: read, no id-token, no secrets, and does not publish; a compromised install reads a public repository. bundler-audit is also the auditing tool itself, so adding it to the SDK's Gemfile would make the gem under audit depend on its own auditor.

This is the one accepted risk, suppressed inline with a justification, matching the fifteen # zizmor: ignore[...] suppressions already in these workflows.

ref-version-mismatch — 3 medium, latent

The trivy-action pin is commented # 0.35.0. That tag exists, so it is valid today and main is not currently flagged.

Upstream stopped publishing unprefixed tags after it: ed142fd is reachable only as v0.36.0, with no 0.36.0. Dependabot mirrors the existing comment style, so when it bumped trivy in #360 it wrote # 0.36.0 — an unknown ref — and the newer zizmor flagged it three times.

Correcting the comment to # v0.35.0 means the next bump inherits the v prefix and resolves on its own. This improves auditability rather than reducing it: the comment exists so a reviewer can check the hash against a real tag.

Verification

Three of the five findings come from zizmor's online audits, which silently do not run without a GitHub token — a plain local run reports clean and looks like a false alarm. All runs below had GH_TOKEN set.

low medium
main today 2 0
#360's head 2 3
this branch 0 0

The 2/3 split on #360's head matches its CI run exactly. The pinned zizmor 1.25.2 that main runs today also still passes here, so the unknown audit name in the ignore comment does not break current CI.

The remaining fifth finding is #360's own trivy comment, which its rebase will regenerate as # v0.36.0 once this lands.

basecamp#360 bumps zizmor-action v0.5.3 -> v0.5.7, which ships a newer zizmor that
reports five findings the pinned version does not. Two are pre-existing here
and one is an artifact of how dependabot writes version comments.

adhoc-packages (2, pre-existing): gem install bundler-audit and npm install -g
npm@latest. Both are deliberate. bundler-audit is the auditing tool itself, and
the npm upgrade is required for OIDC trusted publishing. Suppressed inline with
justifications, matching the fifteen suppressions already in these workflows.

ref-version-mismatch (3, latent): the trivy-action pin is commented # 0.35.0.
That tag exists, so it is valid today, but upstream stopped publishing
unprefixed tags after it -- ed142fd (0.36.0) is only reachable as v0.36.0.
Dependabot copies the existing comment style, so it wrote # 0.36.0 in basecamp#360 and
zizmor flagged an unknown ref. Switching to # v0.35.0 makes the next bump
resolve correctly on its own.

With these in place basecamp#360 should go green on a rebase, with no edits to its
branch.
Copilot AI review requested due to automatic review settings July 10, 2026 03:05
@github-actions

Copy link
Copy Markdown

Sensitive Change Detection (shadow mode)

This PR modifies control-plane files:

  • .github/workflows/release-typescript.yml
  • .github/workflows/security.yml

Shadow mode — this check is informational only. When activated, changes to these paths will require approval from a maintainer.

@github-actions github-actions Bot added the github-actions Pull requests that update GitHub Actions label Jul 10, 2026

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Prepares the repo’s GitHub Actions workflows to pass a newer zizmor version (as part of zizmor-action bump in #360) by aligning Trivy pin comments with upstream tag naming and suppressing two intentional “ad-hoc” package installs.

Changes:

  • Updated aquasecurity/trivy-action pin comments from # 0.35.0# v0.35.0 to avoid ref-version-mismatch findings on future Dependabot bumps.
  • Added inline zizmor: ignore[adhoc-packages] suppressions (with justification) for gem install bundler-audit and npm install -g npm@latest.

Tip

If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
.github/workflows/security.yml Adjusts Trivy pin comment format and suppresses intentional bundler-audit install to satisfy newer zizmor checks.
.github/workflows/release-typescript.yml Suppresses intentional global npm upgrade step to satisfy newer zizmor checks.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 2 files

Re-trigger cubic

The publish job installed npm@latest from the registry, unpinned, inside the
job that holds id-token: write and the release-npm environment, three steps
before npm publish. A compromised install there could publish a malicious
@37signals/basecamp rather than merely read a public repo.

Node 24 bundles npm 11.16, past the 11.5.1 that OIDC trusted publishing
requires, so the upgrade step is unnecessary. Bumping the publish job to Node
24 removes the ad-hoc install entirely: npm now comes from the Node tarball,
which setup-node verifies by checksum.

This removes the finding rather than suppressing it. Verified under Node 24
(npm 11.16.0): npm ci, npm run build (what prepublishOnly runs), and the full
suite, 616 tests across 64 files.
Copilot AI review requested due to automatic review settings July 10, 2026 03:12
@github-actions github-actions Bot added the bug Something isn't working label Jul 10, 2026

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

@robzolkos
robzolkos merged commit f59e2ac into basecamp:main Jul 10, 2026
46 checks passed
jeremy added a commit that referenced this pull request Jul 14, 2026
Main bumped the shared action pins (#360/#365/#367 zizmor prep + dep
bumps) after this workflow was authored: checkout v7.0.0, setup-go
v6.5.0, setup-node v6.4.0, setup-java v5.4.0, setup-gradle v6.2.0,
setup-ruby v1.316.0, setup-uv v8.2.0. Copy test.yml's exact pin lines
so live-canary.yml doesn't reintroduce superseded SHAs.
upload-artifact already matched.
jeremy added a commit that referenced this pull request Jul 22, 2026
Main bumped the shared action pins (#360/#365/#367 zizmor prep + dep
bumps) after this workflow was authored: checkout v7.0.0, setup-go
v6.5.0, setup-node v6.4.0, setup-java v5.4.0, setup-gradle v6.2.0,
setup-ruby v1.316.0, setup-uv v8.2.0. Copy test.yml's exact pin lines
so live-canary.yml doesn't reintroduce superseded SHAs.
upload-artifact already matched.
jeremy added a commit that referenced this pull request Jul 22, 2026
Main bumped the shared action pins (#360/#365/#367 zizmor prep + dep
bumps) after this workflow was authored: checkout v7.0.0, setup-go
v6.5.0, setup-node v6.4.0, setup-java v5.4.0, setup-gradle v6.2.0,
setup-ruby v1.316.0, setup-uv v8.2.0. Copy test.yml's exact pin lines
so live-canary.yml doesn't reintroduce superseded SHAs.
upload-artifact already matched.
jeremy added a commit that referenced this pull request Jul 22, 2026
Main bumped the shared action pins (#360/#365/#367 zizmor prep + dep
bumps) after this workflow was authored: checkout v7.0.0, setup-go
v6.5.0, setup-node v6.4.0, setup-java v5.4.0, setup-gradle v6.2.0,
setup-ruby v1.316.0, setup-uv v8.2.0. Copy test.yml's exact pin lines
so live-canary.yml doesn't reintroduce superseded SHAs.
upload-artifact already matched.
jeremy added a commit that referenced this pull request Jul 22, 2026
…eduled CI (#308)

* conformance: pairwise BC4↔BC5 schema + canonical memories rule

Extends `conformance/schema.json` with `pairwiseAssertions[]` and four
rule types: pairwiseSupersetArray, pairwiseSupersetKeys, pairwiseEqual,
pairwiseDeltaAllowed. The first three enforce the additive-only
invariant between BC4 and BC5 snapshots of the same operation; the
fourth is a per-test allowlist with a required `reason` so a future
public compatibility report can index accepted divergences.

Per-backend schema validation alone can't catch this class of
regression: with every new BC5 field marked optional, a hypothetical
case where BC4 emits `memories: [items]` and BC5 emits `memories: []`
passes each backend's schema independently. The pairwise rules close
that loop.

Path syntax: dotted identifiers relative to each snapshot. The empty
string addresses the body root; `foo.bar` defaults to
`pages[0].body.foo.bar` for single-page snapshots; explicit
`pages[N].body.X` or aggregate `pages[*].body.X` are available when
needed.

Adds the canonical canary rule on `GetMyNotifications`: BC3 commit
64acf34 aliases BC5 memories[] to bubble_ups[] so BC4 API consumers
keep receiving the same population on BC5; the pairwiseSupersetArray
rule fires immediately if that alias ever drops. A pairwiseSupersetKeys
rule on the body root catches any other key removal between backends.

* scripts: pairwise canary comparison

Implements `scripts/compare-canary-runs.sh`, the executable side of the
pairwise rules added in the prior commit. Reads BC4 and BC5 snapshot
directories (one wire snapshot per operation, format established by the
TS live-runner), looks up each test's `pairwiseAssertions` in
`live-my-surface.json`, and applies the four rule types in turn.

Path resolution: `""` → body root, `foo.bar` → `pages[0].body.foo.bar`,
`pages[N].body.X` → explicit page, `pages[*].body.X` → aggregate
across pages (collected into a list, so SupersetArray on the aggregate
checks "number of pages emitting this", not a sum of inner lengths;
that's an intentional semantic — multi-page aggregation is the escape
hatch, single-page page-relative addressing is the common case).

Exit codes: 0 clean, 1 violations, 2 operator error. Violation output
groups by operation and includes the rule + display path so the
specific invariant that fired is obvious without re-running.

Identical account state across the two runs is mandatory for pairwise
results to be meaningful; CONTRIBUTING.md documents this in the
forthcoming docs commit.

* make: conformance-live + check-bc5-compat orchestrators

Adds two opt-in targets that thread the live canary pipeline end-to-end.

`make conformance-live` runs one backend's pass: TS captures canonical
wire snapshots, then Ruby/Python/Go/Kotlin replay-decode them. Requires
LIVE_RECORD_DIR + BASECAMP_BACKEND in the env; not invoked by
`make check`.

`make check-bc5-compat` is the top-level orchestrator: it wipes
LIVE_RECORD_DIR (default `tmp/live-canary`), runs `conformance-live`
once with BASECAMP_BACKEND=bc4, again with BASECAMP_BACKEND=bc5 against
BC5_HOST, then invokes `scripts/compare-canary-runs.sh` on the two
captured snapshot directories. Failure at any stage fails the whole
target.

The TS runner writes to `$LIVE_RECORD_DIR/$BASECAMP_BACKEND/wire/`, so
one LIVE_RECORD_DIR root is reused across both passes and the per-
backend distinction lives in BASECAMP_BACKEND. The plan documents this
explicitly to avoid a separate-dir mental model that would diverge from
what the TS runner actually does.

* ci+docs: live-canary scheduled workflow + CONTRIBUTING pairwise section

Adds `.github/workflows/live-canary.yml` — a nightly cron + manual
workflow_dispatch trigger that runs `make check-bc5-compat`. Gated on
repo secrets (BASECAMP_TOKEN, BASECAMP_ACCOUNT_ID, BC5_HOST); the job
no-ops with a clear log message when the secrets aren't configured
rather than failing the run, so forks and clones without canary access
don't break.

Sets up the full polyglot toolchain mirroring the existing Conformance
Tests job — Go, Node, Ruby, Python, Java/Gradle — since the orchestrator
invokes one capture + four replays. Uploads the entire `tmp/live-canary`
tree as a workflow artifact with 14-day retention so failures can be
inspected post-hoc without rerunning a costly live capture.

CONTRIBUTING.md "Live canary" section gains a "Pairwise BC4↔BC5
comparison" subsection (the four rule types, the path syntax, the
canonical `memories` example), an "Orchestrator" subsection (the make
target wiring with the BC4-then-BC5-then-compare flow), and a
"Scheduled CI" subsection (secret requirements + opt-in behavior). The
stale "PR 4 lands the orchestrator" reference removed; the live infra
is now the documented state, not a forward promise.

Identical account state across the two runs is the load-bearing
operational requirement and is called out explicitly in the docs — the
pairwise rules assume the BC4 and BC5 backends see equivalent fixtures;
without that, `unreads`-style natural drift will false-fail rules.

* docs: drop stale PR-numbered references to canary infra

PR 4 is now shipping the orchestrator + pairwise comparison, so the
forward-looking "PR 3 / PR 4" markers in CONTRIBUTING and COORDINATION
no longer describe pending work — they describe what's in this commit's
parent series. Rewrite the references to name the artifacts directly
(`make conformance-*-replay`, `scripts/compare-canary-runs.sh`,
`make check-bc5-compat`) so the docs stay accurate after PR sequencing
shifts.

* conformance: address PR #308 review threads + reframe memories canary rule

Post-launch reconciliation for the pairwise BC4<->BC5 canary. Clears the 14
review threads on #308 and corrects the memories rule to match what BC5
master actually shipped.

scripts/compare-canary-runs.sh:
- P0 (codex/cubic): match the TS live runner's snapshot filename scheme
  exactly -- sanitize the test name with [^A-Za-z0-9_-]+ -> "_" (case
  preserved), not lowercase/hyphen/operation-first. The old form matched no
  files, so check-bc5-compat could report clean without comparing anything.
- P1 (cubic): a declared pairwise test missing a snapshot on either backend
  is now a hard error (exit 2), not a silent skip-and-pass.
- empty `paths` loop (copilot): guard ${#RULE_PATHS[@]} instead of
  "${RULE_PATHS[@]:-}" so an empty array can't run against the body root.
- pairwiseEqual (cubic): compare via jq deep-equality so object key-order
  differences don't false-fail.
- exit-code contract (copilot): snapshot-read jq failures remap to the
  documented exit 2 instead of leaking jq's status under set -e.

conformance/schema.json: pairwiseAssertions[].paths gains minItems:1.

Makefile:
- rm -rf "$(LIVE_RECORD_DIR)" guarded against empty / "/" / "..".
- conformance-live validates LIVE_RECORD_DIR/BASECAMP_BACKEND before the TS
  live pass (invoked from the recipe after the guards) rather than after it.
- check-bc5-compat comment: "reports all violations", not "fails on first".
- new check-compare-canary target, wired into `make check`.

.github/workflows/live-canary.yml: default BASECAMP_HOST in the workflow
expression so an unset optional secret can't clobber the Makefile default.

CONTRIBUTING.md: align the "all violations" wording; reframe the memories
example to the waiver pattern.

memories canary rule (conformance/tests/live-my-surface.json): the prior
"BC3 commit 64acf34 aliases memories[] to bubble_ups[]" claim is
counterfactual -- that commit does not exist, and BC5 master shipped
`json.memories []` while BC4 four still populates it. The
pairwiseSupersetArray rule now states the intended invariant and is waived by
a temporary pairwiseDeltaAllowed entry pointing at
spec/api-gaps/memories-emptied-regression.md (added on the reconciliation
branch); remove the waiver once BC3 #10947 (which carries
`json.memories @bubble_ups`) merges.

scripts/test-compare-canary-runs.sh: network-free regression coverage for the
filename scheme (incl. proof the old form finds nothing), missing-snapshot
hard-fail, memories-waiver scoping, pairwiseEqual key-order, and the
empty-paths guard.

* conformance: clarify BC4 branch reference in memories waiver reason

The waiver `reason` strings read "BC4 four still populates it", which scans as
a typo to anyone without the post-launch topology in hand (BC4 is the `four`
branch). Reword to "BC4 (the four branch)" in the live-my-surface.json
description + pairwiseDeltaAllowed reason and the CONTRIBUTING.md example.
Addresses the Copilot re-review on PR #308.

* ci(live-canary): align action pins with main's bumped SHAs

Main bumped the shared action pins (#360/#365/#367 zizmor prep + dep
bumps) after this workflow was authored: checkout v7.0.0, setup-go
v6.5.0, setup-node v6.4.0, setup-java v5.4.0, setup-gradle v6.2.0,
setup-ruby v1.316.0, setup-uv v8.2.0. Copy test.yml's exact pin lines
so live-canary.yml doesn't reintroduce superseded SHAs.
upload-artifact already matched.

* conformance: strict pairwiseSupersetKeys types + decouple harness from full fixture

Two Copilot findings on the rebased branch:

* pairwiseSupersetKeys only flagged the BC4-object/BC5-non-object case;
  any other non-null non-object (array/string/number) on either side
  silently did nothing, hiding a mis-specified path or a real
  object → scalar shape change. Null still counts as the empty key set
  (absent-on-one-side stays additive-friendly and a dropped object now
  reports its missing keys); anything else is a violation, mirroring
  pairwiseSupersetArray's INVALID handling. New scenarios G (scalar at
  a keys path fails, no silent skip) and H (BC4 keys vs BC5 null
  reports the missing keys) pin both semantics.

* The regression harness ran scenarios A-D against the entire real
  live-my-surface.json, so adding any future pairwise-bearing test to
  the real fixture would break them with missing-snapshot hard errors
  for tests the harness never writes snapshots for. A-D now run against
  a filtered tests file holding just the verbatim-real
  GetMyNotifications entry (startup asserts exactly one match).

* conformance: fail loud when the pairwise tests file can't be parsed

mapfile -t TEST_ENTRIES < <(jq ...) hid jq failures — a process
substitution's exit status is invisible to set -e, so a malformed tests
file left TEST_ENTRIES empty and the script exited 0 as 'nothing to
compare', silently masking an operator error and breaking the 0/1/2
exit-code contract. Materialize the jq output first and exit 2 with a
parse error when jq fails. Scenario I pins the behavior.

* conformance: reject non-array tests files, fast env preflight, 3.81-safe defaults

Three review threads plus a latent bug the preflight work surfaced:

* The tests-file parser's jq program started with '.[]', which iterates
  object values too — a top-level object was misinterpreted instead of
  treated as an operator error. An explicit type guard now errors inside
  jq (caught by the existing exit-2 path). Scenario J pins it.

* conformance-live preflights BASECAMP_TOKEN and BASECAMP_ACCOUNT_ID in
  the Makefile so a missing credential fails in milliseconds with a
  clear source instead of deep inside the vitest run after npm ci. The
  doc comment no longer lists BASECAMP_LIVE=1 as caller-required — the
  conformance-typescript-live recipe sets it itself.

* check-bc5-compat's target-specific '?=' defaults are gone: GNU make
  3.81 (macOS /usr/bin/make) leaks 'target: VAR ?= default' into a
  global override that clobbers the caller's environment to empty for
  every OTHER target's recipe — with the makefile merely loaded,
  'conformance-live' rejected a perfectly good LIVE_RECORD_DIR from the
  environment. Defaults now resolve in the recipe shell
  (${LIVE_RECORD_DIR:-tmp/live-canary}), which behaves identically on
  make 3.81 and 4.x. Verified under 3.81: conformance-live now sees
  caller env; the rm -rf safety guard still refuses '/' and '..'.

* ci(live-canary): update BASECAMP_HOST default rationale comment

The comment referenced the Makefile's 'BASECAMP_HOST ?=' default, which
the previous commit replaced with recipe-shell ${VAR:-default}
resolution (which also catches the empty-string expansion of an unset
secret). The workflow-level default stays for self-description; the
comment now says why.

* conformance: pages[*] superset arrays compare item totals; bash>=4 guard

Two review threads:

* pairwiseSupersetArray on an aggregated pages[*] path compared the
  outer array length — which is just the page count — so a field with
  arrays on every BC4 page and dropped on every BC5 page false-greened
  whenever page counts matched (codex P2). Aggregated paths now compare
  the TOTAL item count across pages: an absent (null) page value
  contributes 0, any non-null non-array page value poisons the total as
  INVALID. Page redistribution with the total preserved stays green —
  page boundaries aren't contract. Scenarios K (dropped-on-every-page
  fails on totals) and L (redistribution passes) pin both sides.

* Explicit bash >= 4 guard at the top: the script uses mapfile, and
  macOS's /bin/bash 3.2 failed with an opaque 'mapfile: command not
  found' mid-script instead of a clear operator error.

* conformance: materialize remaining jq reads; keep make -n non-destructive

Round follow-ups:

* ALLOW_PATHS, ENFORCED_RULES, and RULE_PATHS got the same
  materialize-before-mapfile treatment as TEST_ENTRIES — a jq failure
  inside a process substitution is invisible to set -e, so extraction
  errors could silently yield empty arrays (skipping the allowlist or
  all rules). Emptiness now drives off jq's own length so the
  empty-string body-root sentinel path ("") is never confused with an
  empty list, with count-mismatch checks failing loud (exit 2).

* check-bc5-compat's rm -rf moved back to its own recipe line: under
  'make -n', lines containing $(MAKE) still execute (with -n
  propagated), so the consolidated block from the 3.81 fix deleted
  snapshots during dry-runs. Guards + rm now sit on a $(MAKE)-free
  line that -n only prints; shell-level defaults are recomputed per
  line. Verified: a marker file under tmp/live-canary survives
  'make -n check-bc5-compat'; the 3.81 env-clobber fix and the rm
  safety guards are unchanged.

* conformance: refuse rm -rf on '.'; explicit error for non-array paths

* The rm -rf guard now strips a trailing slash and refuses '', '.', and
  '/' — LIVE_RECORD_DIR=. (or ./) would have deleted the repo checkout,
  a realistic operator slip for a locally-run target. Verified both
  forms are refused with exit 2.

* A non-array 'paths' on a rule (string or object) now exits 2 with
  "'paths' ... must be an array of strings" instead of leaking jq's
  raw exit status (string case) or silently iterating object values as
  paths (object case, which jq accepts without error). The count
  expression type-guards before the element extraction; scenario M
  pins the contract.

* conformance: preserve trailing body-root paths, validate snapshot shape, full slash-strip

Round follow-ups, all three verified with new harness scenarios:

* Path extraction switched to NUL-delimited jq output (mapfile -d '').
  Newline splitting through command substitution strips trailing blank
  lines, so a valid rule ending with the documented body-root sentinel
  (paths: ["foo", ""]) lost the final path and exit-2'd on the count
  mismatch. NUL delimiters preserve empty-string paths; the count check
  remains as the backstop for jq failures inside the process
  substitution and for non-string elements. Requires bash >= 4.4 for
  mapfile -d (guard bumped from 4.0 with the same clear error).
  Scenarios O (trailing "" evaluated, exit 0) and O2 (its violation
  still fails, exit 1) pin both sides.

* Wire snapshots are validated structurally after the existence check:
  valid JSON alone ('{}', '{"pages":{}}') made every read return null,
  silently false-greening superset rules. The TS runner's contract is
  enforced: object, non-empty pages array, pages_count matches.
  Scenario N pins it.

* The rm -rf guard strips ALL trailing slashes before the case check —
  a single strip let LIVE_RECORD_DIR=/// through as '//', which rm
  resolves to root. Verified /// now refuses with exit 2; the error
  reports the caller's original value.

* conformance: absolutize LIVE_RECORD_DIR, validate waiver types + page shape

Four review threads:

* conformance-live canonicalizes LIVE_RECORD_DIR to an absolute path
  before fanning out (codex P1): the capture and replay recipes each cd
  into their runner directory, so the documented relative default
  tmp/live-canary scattered snapshots under conformance/runner/*/ while
  the pairwise compare read from the repo root — a configured
  check-bc5-compat run failed with missing snapshots instead of
  comparing. Children now receive the absolutized path explicitly.

* A pairwiseDeltaAllowed waiver whose 'paths' is a bare string was
  flattened into a single valid allowed path, silently suppressing the
  enforcing rule. The extraction now requires each waiver's paths to be
  an array of strings and exits 2 otherwise. Scenario P pins it
  (bare-string waiver + real regression → exit 2, not a pass).

* validate_snapshot enforces the documented per-page shape ({status,
  headers, body, bodyText, url}) — pages like [{}] passed the count
  check but read as null, false-greening rules against a damaged
  baseline. Scenario Q pins it.

* The regression harness fails fast with the same bash >= 4.4 guard as
  the compare script, instead of sixteen confusing scenario failures
  under macOS system bash.

* docs: align pages[*] path semantics with the compare implementation

The bullet suggested pages[*] was for how-many-pages checks, but
pairwiseSupersetArray compares the total item count across pages
(absent page values contribute 0, non-array page values are invalid).
Say so, so operators don't write rules against the old semantics.

* conformance: waivers require a non-empty reason at runtime

schema.json requires a reason on pairwiseDeltaAllowed, but the compare
script also runs standalone (check-bc5-compat, the scheduled workflow)
with no schema-validation step in front of it — a reason-less waiver
still flattened into ALLOW_PATHS and suppressed enforcement unaudited.
The extraction now rejects waivers without a non-empty string reason
(exit 2). Scenario R pins it; scenario P's assertion follows the
combined waiver error message.

* conformance: rm -rf -- for option-injection robustness

A LIVE_RECORD_DIR (or, theoretically, a mktemp result) beginning with
'-' would be parsed as an rm option; end option parsing explicitly.

* docs(make): note the bash >= 4.4 requirement on check-compare-canary

The compare + harness scripts guard for it with a clear error; the
Makefile comment now says so up front for contributors running
make check on stock macOS bash.

* conformance: snapshot/test operation integrity; schema forbids empty waiver reason

* Each snapshot's recorded .operation must match the test entry's
  operation before comparison — a stale or overwritten file with the
  right name exits 2 instead of being compared as this test's capture.
  Scenario S pins it.

* conformance/schema.json aligns with the runtime waiver guard: the
  pairwiseDeltaAllowed conditional now sets minLength 1 on reason, so
  an empty-string reason fails schema validation the same way it fails
  the compare script. TS conformance suite re-run green (84 passed).

* conformance: waiver paths must be non-empty at runtime

schema.json's minItems:1 on paths doesn't run in front of standalone
invocations (check-bc5-compat, the scheduled workflow). An empty waiver
paths array is functionally harmless (it waives nothing) but is always
a fixture mistake — reject it with the same exit-2 operator error as
the other waiver shape violations. Scenario T pins it.

* conformance: segment-aware .. guard, jq preflight, broader read-error message

* The rm -rf guard now refuses '..' only as a PATH SEGMENT (leading,
  interior, trailing, or the whole path) instead of any substring, so
  benign names like tmp/live-canary..pr308 are allowed while every
  traversal form still refuses. Patterns unit-verified for all six
  shapes.

* The regression harness preflights jq with the same clear error as
  the compare script instead of failing mid-setup with a raw
  command-not-found.

* snapshot_read_error's hint no longer blames only malformed JSON —
  an invalid jq path expression built from a rule's path entry lands
  in the same handler.

* docs: pairwiseSupersetKeys does not take pages[*] aggregates

The aggregate is a list, not an object — the rule flags it as an
invalid target rather than evaluating it. Say so and point rule
authors at pages[N].body.X for keys checks on paginated endpoints.

* conformance: '[*]' only as the leading pages[*] segment

The aggregate handling keyed off any '[*]' substring, broader than the
documented pages[*].body.X form — an undocumented items[*].foo would
stream through jq with undefined comparison semantics instead of
surfacing the fixture mistake. Both the read and the supersetArray
totals branch now see only validated leading-pages[*] paths; anything
else exits 2 as an unsupported path. Scenario U pins it.

* docs: bash/jq prerequisites; pin double-star rejection

* CONTRIBUTING's prerequisites now list the shared tooling make check
  needs beyond per-SDK toolchains: jq, and bash >= 4.4 for the
  pairwise-canary scripts (macOS /bin/bash is 3.2 — brew install bash;
  the scripts fail fast with that exact hint).

* Scenario U2 pins that a second '[*]' after a valid leading pages[*]
  is rejected as unsupported (the guard already covered it).

* conformance: refuse checkout-ancestor LIVE_RECORD_DIR; mktemp template

* An absolute LIVE_RECORD_DIR equal to the repo checkout (or any of
  its ancestors, e.g. LIVE_RECORD_DIR=$PWD or $HOME) passed the
  guards and rm -rf'd the working tree before the canary ran. A new
  prefix check refuses any path the checkout lives under; absolute
  paths elsewhere (/tmp/canary) and in-checkout subdirs still work.
  Patterns unit-verified for checkout, $HOME, /tmp, relative, and
  in-checkout-absolute shapes.

* The harness's mktemp gets an explicit template — current macOS
  accepts a bare -d (empirically verified here), but older BSD
  variants insist on a template and it costs nothing.

* conformance: reframe memories waiver as permanent documented contract divergence

bc3's BC5 API train shipped 2026-07-18..21 and settled the memories question:
doc/api/sections/my_notifications.md (bc3 #11628) documents memories as an
always-empty placeholder superseded by bubble_ups, and bc3 #10947 — the
once-planned `json.memories @bubble_ups` alias — is closed unmerged. The
pairwiseDeltaAllowed entry is therefore not a temporary waiver pending a fix;
it is the permanent machine-readable record of an accepted BC4→BC5
subtractive delta.

Update the GetMyNotifications test description and both rule reasons in
live-my-surface.json, and align the canonical example + prose in
CONTRIBUTING.md, replacing the 'drop the waiver once #10947 merges' guidance
with the actual retire conditions: BC4 empties memories too, or BC5's
documented contract changes.

* conformance: validate rule paths before applying the waiver skip

is_allowed's early continue ran before the '[*]' syntax guard, so an
unsupported path (items[*].foo, a double star) that also appeared in a
pairwiseDeltaAllowed waiver exited clean instead of reporting the fixture
error — a typo in a waived canary rule could permanently suppress
enforcement without ever surfacing. Validate path syntax first, then apply
the allowlist skip. Adds scenario U3 covering the waived-unsupported-path
case. Flagged by Codex review.

* conformance: reject unknown rule types before applying path waivers

The unknown-RULE_TYPE rejection lived in the dispatch case inside the
per-path loop, which only runs for paths that survive the is_allowed skip.
A misspelled type (pairwiseSupersetAray) whose paths were all waived by
pairwiseDeltaAllowed therefore exited clean — an operator error silently
suppressed, and the typo'd rule never enforced anything again. Hoist the
type check to rule extraction, before any per-path processing; the case's
catch-all arm stays as defense in depth. Adds scenario V covering the
waived-misspelled-type case. Flagged by Codex review.

* conformance: reject all undocumented bracket paths, not just '[*]'

The star guard only matched the literal '[*]', so a bare jq-stream typo
like items[] (or pages[0].body.items[]) reached read_value unwrapped: the
stream made the length variables multi-line/empty, the -lt comparison
errored falsy, and a real shrink violation (BC4 [1,2] vs BC5 []) exited 0
— a false-green on exactly the invariant the script enforces.

Generalize the guard: brackets are only accepted as a leading pages[N] or
pages[*] segment (regex-validated), everything else is rejected as a
fixture mistake with exit 2, still ahead of the waiver skip. Adds
scenario W reproducing the items[] false-green. Flagged by Codex review.

* conformance: harden pairwise path + snapshot validation (review round 4)

- extract validate_rule_path() and apply it to pairwiseDeltaAllowed
  entries too: a malformed bracket path appearing only in a waiver names
  a path no enforcing rule can ever target and previously sat unnoticed
- require bracketed paths to enter the page body (pages[N].body...):
  page-level keys other than body are backend-specific metadata with no
  pairwise semantics, and a typo like pages[0].items read null on both
  snapshots and false-greened superset rules
- validate_snapshot now type-checks page keys (status number, headers
  object, bodyText string, url string) so typed corruption fails loud
  instead of reading as nulls
- check-bc5-compat rm guard: canonicalize an existing LIVE_RECORD_DIR
  (cd + pwd -P) before the checkout-ancestor check so non-canonical
  spellings can't slip past the string compare; a failed cd empties LRD,
  which the ancestor pattern then refuses

Regression scenarios X (waiver-only malformed path), Y (bracket path
skipping .body), Z (wrongly-typed page keys) added; 29/29 green.

* conformance: reject pages[N] rules whose page is absent from a capture

A concrete pages[N] rule asserts the page exists on both backends. When
either capture had fewer pages, read_value returned null and the rule
passed clean — hiding a fixture typo (pages[1] against single-page
captures) or a genuine page-count regression. Both demand operator
attention: fail with exit 2 naming the short snapshot and its page
count. Cross-page-count comparison remains the job of pages[*]
aggregation.

Regression scenarios AA (absent from both) and AB (one-sided page loss,
error names the bc5 snapshot) added; 31/31 green.

* conformance: whitelist rule-path grammar instead of blacklisting brackets

A path with jq punctuation but no brackets ('items, .pages') slipped the
bracket blacklist and interpolated into the jq program as raw syntax.
Terminate the class: validate_rule_path now admits only the documented
grammar — dotted [A-Za-z0-9_-] key segments, optionally led by a
pages[N]/pages[*] indexer that must enter the page body, plus the ""
body-root sentinel — and rejects everything else as a fixture mistake.

Regression scenario AC (comma path) added; 32/32 green.

* conformance: drop hyphens from path grammar; stop uploading raw wire snapshots

- validate_rule_path: remove '-' from the segment charset — '.foo-bar'
  parses in jq as subtraction ('.foo - bar'), not a key lookup, so a
  hyphen-keyed rule evaluated to garbage instead of reading the field.
  No fixture path uses hyphens; keys requiring quoting are a future,
  deliberate extension of to_jq_path. Regression scenario AD added
  (33/33 green).
- live-canary.yml: upload only the decode/ result trees. Wire snapshots
  carry raw response bodies and captured headers from a live account and
  must not persist as downloadable workflow artifacts; decode results
  carry only booleans, error strings, and dotted field paths.

* make: rm the original LIVE_RECORD_DIR path, guard on the canonical one

The round-7 canonicalization rewrote LRD itself, so rm -rf on a
symlinked LIVE_RECORD_DIR deleted the tree the link points to instead
of the link — a behavior change with data-loss potential. Canonicalize
into a separate guard variable for the checkout-ancestor check and keep
the rm on the original (trailing-slash-stripped) path: a symlink is
removed as a link, while a symlink pointing into the checkout is still
refused by the guard (verified both by hand).

* conformance: redact live values from pairwiseEqual violation output

A pairwiseEqual failure printed the complete BC4 and BC5 JSON values
into the comparison output, which the scheduled canary lands in
retained, log-access-readable Actions logs — leaking live-account field
content (names, URLs, notification text) on any drift. Report only
shape summaries (type + keys/length) and point the operator at a local
re-run for raw values. Regression scenario AE proves the violation
still fires while the content stays out of the output; 34/34 green.

* conformance: redact live values from superset invalid-target violations

Completes 5270e94: the invalid-target branches of pairwiseSupersetArray
and pairwiseSupersetKeys still embedded the full BC4/BC5 JSON values in
violation output. Factor the shape summary into a shared shape_of()
helper and use it at all three sites; no violation message carries live
field content anymore (grep-verified zero BC4=$BC4_VAL sites).

* conformance: require explicit page syntax once a capture paginates

Shorthand paths ("" or "foo.bar") normalize to pages[0].body — fine
for single-page captures, but against a multi-page snapshot they
silently checked only page 0 and ignored the rest, under-enforcing the
rule. Hard-error with a pointer to pages[N]/pages[*] syntax as soon as
either side's capture has more than one page. Regression scenario AF
added; 35/35 green.

* conformance: use explicit ERE in scenario X's grep

The \| alternation in a plain grep pattern is a GNU basic-regex
extension; BSD grep treats it literally. grep -E with an unescaped
pipe is unambiguous everywhere.

* make: refuse a LIVE_RECORD_DIR that canonicalizes to filesystem root

A symlink to / (or /.) passed the checkout-ancestor guard by string
accident: the canonical root makes the pattern '//' + '*', which never
matches an absolute pwd. No destructive path actually remained — rm
operates on the original path, so a root symlink lost only the link and
rm refuses trailing-dot operands — but the guard's invariant should be
explicit, not an accident of rm semantics. Refuse a canonical '/'
outright (verified by hand with a symlink to /).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working github-actions Pull requests that update GitHub Actions

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants